fix(install): recover from non-directory entries blocking .bit_roots links - #10355
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens hardLinkDirectory() against corrupted destination paths (e.g., where an expected directory in the destination tree is instead a regular file or a dangling symlink), by quarantining the blocking entry and retrying directory creation so installs/linking can proceed without manual cleanup.
Changes:
- Introduce
ensureDir()+ helpers to recover frommkdir(..., { recursive: true })failures caused by non-directory path entries, by renaming the blocking entry aside and retrying. - Update
hardLinkDirectory()/linkFile()to route directory creation throughensureDir()and normalize errno handling viaerrnoCode(). - Add unit tests covering recovery when destination ancestors/targets are regular files or dangling symlinks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scopes/toolbox/fs/hard-link-directory/hard-link-directory.ts | Adds self-healing directory creation (ensureDir) and wires it into linking flow; adds warnings via legacy logger. |
| scopes/toolbox/fs/hard-link-directory/hard-link-directory.spec.ts | Adds regression tests for recovery/quarantine behavior under corrupted destination layouts. |
…abled (#10356) ## Summary The post-install path runs through `linkCodemods` → `linkToNodeModulesByIds` → `NodeModuleLinker.link()`, and that ended with an **unconditional** call to `linkPkgsToRootComponents` at `node-modules-linker.ts:73`. The sibling call site in `install.main.runtime.ts` (`_linkAllComponentsToBitRoots`, line 1272) is already gated on `dependencyResolver.hasRootComponents()` — this aligns the linker with that. ### When this matters A user toggles `rootComponents` from `true` to `false` and runs `bit install`: 1. Prior install (with `rootComponents: true`) populated `node_modules/.bit_roots/<env>/node_modules/...`. 2. New install (with `rootComponents: false`): pnpm no longer treats `.bit_roots/<env>` as a workspace project, and `_updateRootDirs` is skipped, so that subtree is no longer managed by anyone. Its internal layout can drift — e.g. ancestors of `<env>/node_modules/<pkg>` may have been reshaped, leaving a regular file or a broken link where a directory used to be. 3. `NodeModuleLinker.link()` then tries to hard-link the workspace's `node_modules/<pkg>` into that stale tree, and `mkdir(... { recursive: true })` throws `ENOTDIR` (or `ENOENT` through a broken symlink). The whole install aborts. Concrete report from a user: ``` ✔ done running package installation using pnpm (completed in 5s) ✔ running post install subscribers ENOTDIR: not a directory, mkdir '/home/user/hope-mobile/node_modules/.bit_roots/bitdev.react-native_react-native-env@2.0.0/node_modules/@teambit/hope.hope-mobile' ``` ### Changes - `Workspace.hasRootComponents()` — small public method that delegates to the private `dependencyResolver.hasRootComponents()`. The linker (and any other consumer of `Workspace`) can now check this without reaching into the private resolver. - `NodeModuleLinker.link()` — gates the `linkPkgsToRootComponents` call on `workspace.hasRootComponents()`. When root components are off, the stale `.bit_roots` tree is left strictly alone. ### Notes - The defensive recovery in #10355 turns the underlying ENOTDIR into a recoverable warning when it does happen. This PR is the upstream fix that prevents bit from touching `.bit_roots` in the first place when it shouldn't. The two are complementary — happy to land in either order. - This PR does not clean up an existing `.bit_roots` when the user toggles to `false`. The stale tree is wasted disk but no longer actively harmful once we stop writing to it. If you want bit to also remove it on toggle, that's a separate change worth thinking through (someone may have tooling that reads from there independently). ## Test plan - [x] `npm run lint` — same 38 pre-existing TS errors as master (all in unrelated `@pnpm/*` imports), no new errors in the changed files - [ ] e2e test — the existing `e2e/harmony/root-components.e2e.ts` is the natural home for a "toggle off and reinstall" scenario but I held off on adding one in this draft; let me know if you want it bundled in.
b415bd7 to
c57d3c5
Compare
Code Review by Qodo
1. Recovery moves arbitrary path entries
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 406f48b |
…links
`hardLinkDirectory` is invoked during post-install linking into
`node_modules/.bit_roots/<env>/...`. If a previous install was interrupted
or the env layout drifted across versions, an ancestor directory in the
target path can be left behind as a regular file or a dangling symlink.
That made `mkdir(... { recursive: true })` throw `ENOTDIR` (or `ENOENT`
through a broken symlink) and aborted the whole install with no clear
remediation other than `rm -rf node_modules/.bit_roots`.
Detect this case, remove the offending non-directory entry, retry the
mkdir, and surface a warning. The destination tree under `.bit_roots` is
owned by bit and rebuilt on every install, so deleting a stray entry is
safe.
Use printWarning + logger.warn from @teambit/legacy.logger as the default onWarn for hardLinkDirectory, so the recovery message both surfaces in the CLI (yellow "Warning: …", honoring no_warnings config) and lands in debug.log alongside the install context. Tests still inject their own collector, so this stays unit-testable without touching the global logger.
Always go through bit's logger (logger.warn + printWarning). The option existed only to keep tests from depending on the global logger, but the recovery contract is sufficiently verified by asserting the file gets linked through — the warning is a side effect, not the behavior under test.
Centralize the cast to NodeJS.ErrnoException in a tiny helper so each catch site stays an unknown without sprinkling `as any` around.
The blocking entry could be high up the path (a stray file at @scope, or even at node_modules itself in a weird state) and we don't want to discard the user's data on a heuristic. Rename it to <offender>.bit-stray-<ts> alongside, surface that path in the warning, and let the user inspect or remove it themselves. The retry mkdir then succeeds because the original name is free.
406f48b to
e23ed75
Compare
|
Code review by qodo was updated up to the latest commit e23ed75 |
| // Another worker may have already moved the offender. Retry mkdir against the new state. | ||
| continue; | ||
| } | ||
| const quarantined = await quarantineStrayEntry(offender); |
There was a problem hiding this comment.
1. Recovery moves arbitrary path entries 🐞 Bug ⛨ Security
ensureDir quarantines blockers without checking that they belong to a generated Bit directory, while injected destinations can be arbitrary absolute paths read from package-manager metadata. A stale or malformed injected location can therefore cause an unrelated file or symlink outside the workspace to be moved during compilation.
Agent Prompt
## Issue description
The new recovery path can rename a blocking entry at any absolute destination supplied to `hardLinkDirectory`. Package-manager metadata can supply absolute injected locations without containment validation, allowing unrelated filesystem entries to be moved.
## Issue Context
The compiler preserves absolute injected paths, and Yarn forwards locations from `.yarn-state.yml`. Recovery should only mutate generated directories explicitly owned by Bit.
## Fix Focus Areas
- scopes/toolbox/fs/hard-link-directory/hard-link-directory.ts[115-145]
- scopes/compilation/compiler/compiler.task.ts[64-83]
- scopes/dependencies/yarn/yarn.package-manager.ts[492-510]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const quarantined = path.join(quarantineDir, path.basename(offender)); | ||
| try { | ||
| await fs.rename(offender, quarantined); | ||
| return quarantined; |
There was a problem hiding this comment.
2. Quarantine retargets relative symlinks 🐞 Bug ≡ Correctness
Quarantine moves an offending symlink from its original parent into a nested directory, so any relative link target is subsequently resolved from a different directory. The entry is retained, but it no longer references the same path and cannot be restored later with its original behavior.
Agent Prompt
## Issue description
Moving a relative symlink into `<offender>.bit-stray-*/<basename>` changes the base directory used to resolve its target. Quarantining should preserve the symlink's effective target as well as its link text and type.
## Issue Context
The existing symlink test uses an absolute target, so it does not detect this behavior. Add coverage for a relative dangling symlink and a relative symlink targeting a non-directory entry.
## Fix Focus Areas
- scopes/toolbox/fs/hard-link-directory/hard-link-directory.ts[157-179]
- scopes/toolbox/fs/hard-link-directory/hard-link-directory.spec.ts[190-214]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ed8e22d |
Summary
hardLinkDirectoryis invoked during post-install linking intonode_modules/.bit_roots/<env>/.... If a previous install was interrupted or the env layout drifted across versions, an ancestor directory in the target path can be left behind as a regular file or a dangling symlink. That makesfs.mkdir(..., { recursive: true })throwENOTDIR(orENOENTthrough a broken symlink) and aborts the whole install with no clear remediation other than deleting.bit_roots.A user hit this on
bit install:This PR makes the linker self-heal in that situation:
ensureDirhelper handlesENOTDIR,EEXIST, andENOENTfromfs.mkdir. It walks up the path withlstat, finds the deepest existing ancestor that is not a directory, and moves it aside rather than deleting it.<offender>.bit-stray-<timestamp>and atomically renames the blocker into<quarantine-dir>/<basename>. The reserved directory prevents destination clobbering; the single rename prevents replacement races and preserves files, symlinks, and Windows junctions without recreating them.ENOENTretry directory creation. Timestamp collisions are suffix-bumped without overwriting an earlier quarantine.logger.warnand displayed through the sharedformatWarningSummaryCLI formatter, while honoringno_warningsand disabled-console modes. It includes both the original and quarantine paths for manual inspection.The change also replaces
catch (err: any)withcatch (err)and a smallerrnoCode(err: unknown)helper.Test plan
git diff --check